Skip to content

Implement ISOSDacInterface::GetLoaderAllocatorHeapNames and GetLoaderAllocatorHeaps in cDAC SOSDacImpl#124818

Open
Copilot wants to merge 12 commits intomainfrom
copilot/implement-get-loader-allocator-heap-names
Open

Implement ISOSDacInterface::GetLoaderAllocatorHeapNames and GetLoaderAllocatorHeaps in cDAC SOSDacImpl#124818
Copilot wants to merge 12 commits intomainfrom
copilot/implement-get-loader-allocator-heap-names

Conversation

Copy link
Contributor

Copilot AI commented Feb 24, 2026

Description

Implements ISOSDacInterface13.GetLoaderAllocatorHeapNames and ISOSDacInterface13.GetLoaderAllocatorHeaps in the managed cDAC SOSDacImpl.cs, replacing the legacy DAC delegation stubs. Based on ClrDataAccess::GetLoaderAllocatorHeapNames and ClrDataAccess::GetLoaderAllocatorHeaps in request.cpp.

Native Data Descriptors

  • Added heap fields to cdac_data<LoaderAllocator> in loaderallocator.hpp: StaticsHeap, ExecutableHeap, FixupPrecodeHeap (#ifdef HAS_FIXUP_PRECODE), NewStubPrecodeHeap (#ifndef FEATURE_PORTABLE_ENTRYPOINTS), DynamicHelpersStubHeap (FEATURE_READYTORUN && FEATURE_STUBPRECODE_DYNAMIC_HELPERS), VirtualCallStubManager
  • Added new cdac_data<VirtualCallStubManager> in virtualcallstub.h: IndcellHeap (unconditional), CacheEntryHeap (#ifdef FEATURE_VIRTUAL_STUB_DISPATCH)
  • Added corresponding entries to datadescriptor.inc with matching feature guards
  • Added VirtualCallStubManager to DataType enum
  • Added #include "virtualcallstub.h" to datadescriptor.h so cdac_data<VirtualCallStubManager> is visible when datadescriptor.inc is compiled

Managed Data Classes

  • Updated Data/LoaderAllocator.cs with optional heap fields (FixupPrecodeHeap, NewStubPrecodeHeap, DynamicHelpersStubHeap as nullable) and always-present fields (StaticsHeap, ExecutableHeap, VirtualCallStubManager)
  • Created Data/VirtualCallStubManager.cs with IndcellHeap and optional CacheEntryHeap

ILoader Contract

  • Added single GetLoaderAllocatorHeaps(TargetPointer) method to ILoader interface returning IReadOnlyDictionary<string, TargetPointer> — dictionary keys are heap names, values are heap pointers
  • Implemented in Loader_1.cs with dynamic dictionary building based on data descriptor field presence via ContainsKey checks, matching the native #ifdef conditional compilation
  • Dictionary keys use nameof(Data.LoaderAllocator.X) and nameof(Data.VirtualCallStubManager.X) instead of hardcoded strings
  • Feature-conditional heaps (FixupPrecodeHeap, NewStubPrecodeHeap, DynamicHelpersStubHeap) are only included when the data descriptor field exists
  • VirtualCallStubManager heaps (IndcellHeap, CacheEntryHeap) are excluded entirely when VirtualCallStubManager is null

SOSDacImpl

  • GetLoaderAllocatorHeapNames: Queries global loader allocator via ILoader.GetLoaderAllocatorHeaps to get heap names from dictionary keys. Lazily initializes ANSI string pointers via Marshal.StringToHGlobalAnsi, cached on the instance. Standard cDAC pattern with try/catch, S_FALSE when buffer is insufficient
  • GetLoaderAllocatorHeaps: Reads heap pointers through the ILoader contract dictionary, fills LoaderHeapKindNormal for all heaps
  • Both methods include #if DEBUG validation blocks that compare cDAC results against legacy DAC output — heap name strings and heap pointer/kind values are validated to match, following the pattern used by other cDAC APIs

Contract Documentation

  • Updated docs/design/datacontracts/Loader.md with:
    • New GetLoaderAllocatorHeaps API in the contract API section
    • New data descriptor fields for LoaderAllocator (StaticsHeap, ExecutableHeap, FixupPrecodeHeap, NewStubPrecodeHeap, DynamicHelpersStubHeap, VirtualCallStubManager) and new VirtualCallStubManager type (IndcellHeap, CacheEntryHeap) in the data descriptors table
    • Pseudocode implementation showing feature-conditional dictionary building logic

Tests

  • 8 test methods in LoaderTests.cs × 4 architectures = 32 new test cases
    • GetLoaderAllocatorHeapNames: Count query, full name retrieval, insufficient buffer, null pNeeded
    • GetLoaderAllocatorHeaps: Count query, full heap retrieval, insufficient buffer, null address
  • All 900 cDAC tests pass

💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

@github-actions github-actions bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Feb 24, 2026
Copilot AI and others added 2 commits February 24, 2026 20:45
Co-authored-by: max-charlamb <44248479+max-charlamb@users.noreply.github.com>
Co-authored-by: max-charlamb <44248479+max-charlamb@users.noreply.github.com>
Copilot AI changed the title [WIP] Implement GetLoaderAllocatorHeapNames in SOSDacImpl.cs Implement ISOSDacInterface::GetLoaderAllocatorHeapNames in cDAC SOSDacImpl Feb 24, 2026
@max-charlamb max-charlamb added area-Diagnostics-coreclr and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Feb 24, 2026
@dotnet-policy-service
Copy link
Contributor

Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag
See info in area-owners.md if you want to be subscribed.

Co-authored-by: max-charlamb <44248479+max-charlamb@users.noreply.github.com>
@max-charlamb
Copy link
Member

@copilot, try again

…dd missing using

Co-authored-by: max-charlamb <44248479+max-charlamb@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Feb 24, 2026

@copilot, try again

Implementation reworked in commits up to 0554c43. Changes:

Feature-conditional heap names: GetLoaderAllocatorHeapNames in Loader_1.cs dynamically builds the names list by checking ContainsKey on the data descriptor fields - DynamicHelpersStubHeap is conditional on FEATURE_READYTORUN && FEATURE_STUBPRECODE_DYNAMIC_HELPERS, and CacheEntryHeap is conditional on FEATURE_VIRTUAL_STUB_DISPATCH, matching the native LoaderAllocatorLoaderHeapNames array.

GetLoaderAllocatorHeaps: Fully implemented in SOSDacImpl.cs, reading heap pointers through the ILoader contract. The contract reads from LoaderAllocator and VirtualCallStubManager data classes, handling null VcsManager by padding with TargetPointer.Null.

Native data descriptors: Added cdac_data<LoaderAllocator> fields (StaticsHeap, ExecutableHeap, FixupPrecodeHeap, NewStubPrecodeHeap, DynamicHelpersStubHeap, VirtualCallStubManager) and new cdac_data<VirtualCallStubManager> (IndcellHeap, CacheEntryHeap) in loaderallocator.hpp, virtualcallstub.h, and datadescriptor.inc.

Tests: 32 new test cases (8 test methods × 4 architectures) covering both methods. All 864 cDAC tests pass.

Copilot AI changed the title Implement ISOSDacInterface::GetLoaderAllocatorHeapNames in cDAC SOSDacImpl Implement ISOSDacInterface::GetLoaderAllocatorHeapNames and GetLoaderAllocatorHeaps in cDAC SOSDacImpl Feb 24, 2026
Copy link
Member

@max-charlamb max-charlamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot , address feedback

…ps API

Replace separate GetLoaderAllocatorHeapNames() and GetLoaderAllocatorHeaps(TargetPointer)
with a single GetLoaderAllocatorHeaps(TargetPointer) returning IReadOnlyDictionary<string, TargetPointer>.

SOSDacImpl.GetLoaderAllocatorHeapNames now queries the global loader allocator to get
names from the dictionary. GetLoaderAllocatorHeaps iterates dictionary values.

Tests updated to use dictionary-based mock and verify unordered name/address sets.

Co-authored-by: max-charlamb <44248479+max-charlamb@users.noreply.github.com>
Copy link
Member

@max-charlamb max-charlamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot , address feedback

…tionary

- Use nameof(Data.LoaderAllocator.X) instead of hardcoded strings for all
  dictionary keys in GetLoaderAllocatorHeaps
- Make FixupPrecodeHeap and NewStubPrecodeHeap conditional on ContainsKey
  (exclude from dictionary when field absent from data descriptor)
- Remove else branch that padded IndcellHeap/CacheEntryHeap with null when
  VirtualCallStubManager is null - only include them when VcsManager exists
- All 864 cDAC tests pass

Co-authored-by: max-charlamb <44248479+max-charlamb@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Feb 24, 2026

@copilot , address feedback

Addressed all three review comments in e0e9cd5nameof for dict keys, conditional inclusion for FixupPrecodeHeap/NewStubPrecodeHeap, and no null-padding for VcsManager heaps. All 864 tests pass.

@copilot, try again

Addressed all three review comments in e0e9cd5nameof for dict keys, conditional inclusion for FixupPrecodeHeap/NewStubPrecodeHeap, and no null-padding for VcsManager heaps. All 864 tests pass.

@max-charlamb
Copy link
Member

@copilot , please fix build break

datadescriptor.h did not include virtualcallstub.h, so the
cdac_data<VirtualCallStubManager> template specialization was not visible
when datadescriptor.inc was compiled. This caused a build failure
referencing cdac_data<VirtualCallStubManager>::IndcellHeap and
cdac_data<VirtualCallStubManager>::CacheEntryHeap.

Co-authored-by: max-charlamb <44248479+max-charlamb@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Feb 24, 2026

@copilot , please fix build break

Fixed in 3f63f2f. The issue was that datadescriptor.h did not include virtualcallstub.h, so cdac_data<VirtualCallStubManager> was not visible when datadescriptor.inc was compiled. Added #include "virtualcallstub.h" to datadescriptor.h.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • https://api.github.com/graphql
    • Triggering command: /usr/bin/gh gh pr checks 124818 --json name,state,link (http block)
    • Triggering command: /usr/bin/gh /usr/bin/gh pr checks 124818 --repo dotnet/runtime (http block)
    • Triggering command: /usr/bin/gh gh pr view 124818 --json statusCheckRollup --jq .statusCheckRollup[] | select(.state == &#34;FAILURE&#34; or .state == &#34;ERROR&#34;) | {name: .name, state: .state, link: .targetUrl} (http block)

If you need me to access, download, or install something from one of these locations, you can either:

@max-charlamb
Copy link
Member

@copilot, address feedback from Noah. We should document any new datadescriptors and APIs in the relevant cDAC markdown file.

Copilot AI and others added 2 commits February 25, 2026 14:59
Add documentation for the new ILoader.GetLoaderAllocatorHeaps API, including:
- New API in the contract API section
- New data descriptor fields for LoaderAllocator (StaticsHeap, ExecutableHeap,
  FixupPrecodeHeap, NewStubPrecodeHeap, DynamicHelpersStubHeap,
  VirtualCallStubManager) and VirtualCallStubManager (IndcellHeap,
  CacheEntryHeap) in the data descriptors table
- Pseudocode implementation showing feature-conditional dictionary building

Co-authored-by: max-charlamb <44248479+max-charlamb@users.noreply.github.com>
…nager and PortableEntryPoint

Co-authored-by: max-charlamb <44248479+max-charlamb@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Feb 25, 2026

@copilot, address feedback from Noah. We should document any new datadescriptors and APIs in the relevant cDAC markdown file.

Documented in ab7f065. Added to docs/design/datacontracts/Loader.md:

  • GetLoaderAllocatorHeaps API in the contract API section
  • New LoaderAllocator data descriptor fields (StaticsHeap, ExecutableHeap, FixupPrecodeHeap, NewStubPrecodeHeap, DynamicHelpersStubHeap, VirtualCallStubManager) and new VirtualCallStubManager type (IndcellHeap, CacheEntryHeap) in the data descriptors table
  • Pseudocode implementation showing feature-conditional dictionary building

Also resolved a merge conflict in DataType.cs in a54d098 (both VirtualCallStubManager and PortableEntryPoint needed). All 900 cDAC tests pass.

Copy link
Member

@max-charlamb max-charlamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot , address feedback

…AllocatorHeaps

Expand #if DEBUG validation blocks to compare actual name strings and
heap pointer/kind values between cDAC and legacy DAC, following the
pattern used by GetAssemblyList and GetMethodsWithProfilerModifiedIL.

Co-authored-by: max-charlamb <44248479+max-charlamb@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Feb 25, 2026

@copilot , address feedback

Addressed in 2c1ed8a. Both GetLoaderAllocatorHeapNames and GetLoaderAllocatorHeaps now validate actual names and heap pointer/kind values against the legacy DAC in the #if DEBUG blocks. All 900 tests pass.

@max-charlamb max-charlamb marked this pull request as ready for review February 25, 2026 21:44
Copilot AI review requested due to automatic review settings February 25, 2026 21:44
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR implements ISOSDacInterface13.GetLoaderAllocatorHeapNames and ISOSDacInterface13.GetLoaderAllocatorHeaps in the managed cDAC, replacing legacy DAC delegation stubs. The implementation adds native data descriptors for loader allocator heaps and VirtualCallStubManager heaps, creates corresponding managed data classes, extends the ILoader contract, and includes comprehensive tests.

Changes:

  • Added native data descriptors for LoaderAllocator heaps (StaticsHeap, ExecutableHeap, and feature-conditional heaps) and new VirtualCallStubManager type with heap fields
  • Implemented GetLoaderAllocatorHeaps in the ILoader contract returning a dictionary of heap names to pointers, with dynamic inclusion based on data descriptor field presence
  • Implemented SOSDacImpl methods to query heap names and addresses through the ILoader contract, with DEBUG validation against legacy DAC

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
src/coreclr/vm/loaderallocator.hpp Added cdac_data fields for StaticsHeap, ExecutableHeap, and feature-conditional heap fields (FixupPrecodeHeap, NewStubPrecodeHeap, DynamicHelpersStubHeap, VirtualCallStubManager)
src/coreclr/vm/virtualcallstub.h Created cdac_data specialization for VirtualCallStubManager with IndcellHeap and CacheEntryHeap fields
src/coreclr/vm/datadescriptor/datadescriptor.h Added #include for virtualcallstub.h to expose VirtualCallStubManager data template
src/coreclr/vm/datadescriptor/datadescriptor.inc Added LoaderAllocator heap fields and new VirtualCallStubManager type with matching feature guards
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/DataType.cs Added VirtualCallStubManager enum value
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/ILoader.cs Added GetLoaderAllocatorHeaps method to ILoader interface
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/LoaderAllocator.cs Added StaticsHeap, ExecutableHeap, and nullable feature-conditional heap properties, plus VirtualCallStubManager pointer
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/VirtualCallStubManager.cs Created new data class with IndcellHeap and nullable CacheEntryHeap properties
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Loader_1.cs Implemented GetLoaderAllocatorHeaps with dictionary building logic that conditionally includes heaps based on data descriptor field presence
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/SOSDacImpl.cs Implemented GetLoaderAllocatorHeapNames and GetLoaderAllocatorHeaps with lazy initialization, caching, and DEBUG validation
src/native/managed/cdac/tests/LoaderTests.cs Added 8 test methods covering count queries, full retrieval, insufficient buffers, and null parameters for both APIs
docs/design/datacontracts/Loader.md Documented new API, data descriptor fields, and pseudocode implementation

Comment on lines +4463 to +4481
private nint[]? _loaderAllocatorHeapNamePtrs;

private nint[] GetOrInitializeHeapNamePtrs()
{
if (_loaderAllocatorHeapNamePtrs is not null)
return _loaderAllocatorHeapNamePtrs;

Contracts.ILoader contract = _target.Contracts.Loader;
TargetPointer globalLoaderAllocator = contract.GetGlobalLoaderAllocator();
IReadOnlyDictionary<string, TargetPointer> heaps = contract.GetLoaderAllocatorHeaps(globalLoaderAllocator);
nint[] ptrs = new nint[heaps.Count];
int i = 0;
foreach (string name in heaps.Keys)
{
ptrs[i++] = Marshal.StringToHGlobalAnsi(name);
}
_loaderAllocatorHeapNamePtrs = ptrs;
return ptrs;
}
Copy link

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Memory allocated by Marshal.StringToHGlobalAnsi is never freed, causing a memory leak. The cached _loaderAllocatorHeapNamePtrs array holds unmanaged memory pointers that persist for the lifetime of the SOSDacImpl instance, but there's no cleanup mechanism (no finalizer, IDisposable implementation, or cleanup in Flush method). Consider adding cleanup logic to free the allocated memory, such as implementing IDisposable or adding cleanup to the Flush method.

Copilot uses AI. Check for mistakes.
Comment on lines +4553 to +4555
foreach (TargetPointer heap in heaps.Values)
{
pLoaderHeaps[i] = heap.ToClrDataAddress(_target);
Copy link

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dictionary enumeration order is not guaranteed to be consistent, which causes a critical mismatch between GetLoaderAllocatorHeapNames and GetLoaderAllocatorHeaps. The native implementation uses a fixed-order static array (LoaderAllocatorLoaderHeapNames in request.cpp lines 3683-3699) that guarantees the heap names at index i correspond to the heap addresses at index i. The cDAC implementation iterates over heaps.Keys in GetLoaderAllocatorHeapNames (line 4475) and heaps.Values in GetLoaderAllocatorHeaps (line 4553), but Dictionary iteration order is not guaranteed to match between Keys and Values enumerations across different API calls or even within the same call in some runtime scenarios. This breaks the API contract where consumers expect names[i] to correspond to heaps[i]. Consider using an ordered collection (e.g., List of KeyValuePair or a custom ordered structure) or ensuring consistent ordering by sorting both Keys and Values by key name.

Suggested change
foreach (TargetPointer heap in heaps.Values)
{
pLoaderHeaps[i] = heap.ToClrDataAddress(_target);
foreach (KeyValuePair<string, TargetPointer> entry in heaps.OrderBy(entry => entry.Key))
{
pLoaderHeaps[i] = entry.Value.ToClrDataAddress(_target);

Copilot uses AI. Check for mistakes.
Comment on lines +129 to +148
public void GetLoaderAllocatorHeapNames_GetNames(MockTarget.Architecture arch)
{
ISOSDacInterface13 impl = CreateSOSDacImplForHeapTests(arch);

int needed;
int hr = impl.GetLoaderAllocatorHeapNames(0, null, &needed);
Assert.Equal(MockHeapDictionary.Count, needed);

char** names = stackalloc char*[needed];
hr = impl.GetLoaderAllocatorHeapNames(needed, names, &needed);

Assert.Equal(HResults.S_OK, hr);
Assert.Equal(MockHeapDictionary.Count, needed);
HashSet<string> expectedNames = new(MockHeapDictionary.Keys);
for (int i = 0; i < needed; i++)
{
string actual = Marshal.PtrToStringAnsi((nint)names[i])!;
Assert.Contains(actual, expectedNames);
}
}
Copy link

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests verify that heap names exist in the expected set but don't verify the ordering correspondence between GetLoaderAllocatorHeapNames and GetLoaderAllocatorHeaps. The legacy DAC contract requires that names[i] corresponds to heaps[i] for the same index i. Consider adding a test that calls both APIs and verifies that the name at each index matches the expected name for the heap address at that index, ensuring the ordering contract is preserved.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants